Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Enter key to select recent DB on OS X #2331

Conversation

gshakhn
Copy link
Contributor

@gshakhn gshakhn commented Sep 27, 2018

Description

Override keyPressEventon WelcomeWidget on OS X to openDatabaseFromFile.

Motivation and context

I work with a few different password DBs. I like to use the keyboard heavily in KeePassXC. I can close an existing DB with Ctrl+W, but I have to use the mouse to open a recent one. Hitting Enter does not work on OS X. Details are below.

My C++ is extremely rusty, so any feedback would be appreciated.

openDatabaseFromFile is already invoked via the QListWidget::itemActivated signal, but this signal doesn't fire on OS X for Enter. QListWidget::itemActivated activates on an OS specific activation key. [1] On Windows/X11, this is Enter, which lets the user easily navigate with just the keyboard. On OS X, this is Ctrl+O, which is already bound to Open Database. This means that itemActivated cannot fire via the keyboard.

Per StackOverflow [2], a recommended solution is to catch the enter/return key press manually.

This seems like a common problem with Qt. [3] [4] I'm guessing other QListWidgets also exhibit this problem, but I don't use them enough for it to be a pain point. This PR fixes this particular widget, but there is likely a more holistic approach for the other widgets.

[1] https://doc.qt.io/archives/qt-4.8/qlistwidget.html#itemActivated
[2] https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac
[3] https://forum.qt.io/topic/36147/pyside-itemactivated-not-triggered-on-mac-os-x-with-return-key
[4] dolphin-emu/dolphin#6099

How has this been tested?

Manually tested on OS X 10.11.6 with Qt 5.11.2. I have not tested on a Windows/X11 environment to verify that the #ifdef is working.

Screenshots (if appropriate):

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)=

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ❌ My code follows the code style of this project. [REQUIRED] (I was unable to run make format. Code change is small and looks like the other code as far as I can tell though)
  • ❌ All new and existing tests passed. [REQUIRED] (the testgui tests are hanging on my machine on the develop branch. I'm not sure if I broke them on this branch, so going to wait for the CI check instead.
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@gshakhn
Copy link
Contributor Author

gshakhn commented Sep 27, 2018

The build failed since keyPressEvent only does something on OS X. Would the recommended fix be to just override it only on OS X instead of leaving it empty for the other OSs?

@droidmonkey
Copy link
Member

This would be better handled if you just gave the selection box the focus when the WelcomeWidget is activated. Plus, don't limit this to just OSX, we only do platform specific code if it is actually platform specific.

@gshakhn
Copy link
Contributor Author

gshakhn commented Sep 27, 2018

What do you mean giving the selection box focus? I can select things in the list with arrow keys, but Enter does not open the DB.

I have not tested this on other OSes, but the Qt docs claim that Enter will activate things on Windows/X11. I believe this is an OS X specific problem.

@gshakhn
Copy link
Contributor Author

gshakhn commented Sep 27, 2018

Looks like the Windows build passed. Code change should have been identical for Windows and Linux. Guessing the Windows build isn't as strict regarding warnings.

@gshakhn
Copy link
Contributor Author

gshakhn commented Sep 27, 2018

I was considering getting rid of the #ifdef and always calling openDatabaseFromFile regardless of OS, but I'm not sure if that method would fire twice on Windows/X11 (once from the signal, once from key handle) and how problematic that would be. I don't have those systems to test on, so I figured that doing the #ifdef was the safest approach. If someone can test on those OSes and verify that the double trigger is not problematic, getting rid of the #ifdef would simplify the code.

@gshakhn
Copy link
Contributor Author

gshakhn commented Sep 27, 2018

Just realized this broke tabbing over to a button (e.g. Create new database) and pressing Enter. Overriding keyPressEvent on WelcomeWidget was a bit broad. Proper fix may involve subclassing QListWidget and overriding keyPressEvent there.

@droidmonkey
Copy link
Member

Please don't spend time on this until I dive into the documentation.

@phoerious
Copy link
Member

Guessing the Windows build isn't as strict regarding warnings.

Windows does release builds only, which don't have -Werror set.

@droidmonkey
Copy link
Member

@gshakhn you need to call down into the baseclass keyPressEvent so everything else works. Do this outside of the Q_MAC_OS guards.

* Override `keyPressEvent`on WelcomeWidget on OS X to `openDatabaseFromFile`.

openDatabaseFromFile is already invoked via the QListWidget::itemActivated signal,
  but this signal doesn't fire on OS X for Enter.
QListWidget::itemActivated activates on an OS specific activation key. [1]
On Windows/X11, this is Enter, which lets the user easily
  navigate with just the keyboard.
On OS X, this is Ctrl+O, which is already bound to Open Database. This means that itemActivated cannot fire via the keyboard.

Per StackOverflow [2], the recommended solution is to catch
the enter/return key press manually.

This seems like a common problem with Qt. [3] [4]

[1] https://doc.qt.io/archives/qt-4.8/qlistwidget.html#itemActivated
[2] https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac
[3] https://forum.qt.io/topic/36147/pyside-itemactivated-not-triggered-on-mac-os-x-with-return-key
[4] dolphin-emu/dolphin#6099
@droidmonkey droidmonkey force-pushed the feature/enter-selects-recent-db-on-osx branch from f0716d3 to 129c2cd Compare December 25, 2018 20:31
@droidmonkey droidmonkey added this to the v2.4.0 milestone Dec 25, 2018
@droidmonkey droidmonkey merged commit b1ff346 into keepassxreboot:develop Dec 25, 2018
droidmonkey added a commit that referenced this pull request Mar 19, 2019
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants